Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve constant expression evaluator #225

Merged
merged 4 commits into from
Oct 13, 2023
Merged

Improve constant expression evaluator #225

merged 4 commits into from
Oct 13, 2023

Conversation

blitz-1306
Copy link
Contributor

@blitz-1306 blitz-1306 commented Oct 6, 2023 •
edited
Loading

Tasks

This PR resolves #224 and fixes #227.

Changes

  • Tweak logic for better constant expression evaluation (isConstant(), evalConstantExpr()):
    • respect file-level constants;
    • support more node types (MemberAccess, IndexAccess VariableDeclaration);
    • support more cast operations (string <-> bytes, string / bytes -> fixed bytes, uint <-> fixed bytes);
  • Tweaked existing unit test to respect changes.
  • Introduced integration test to have an ability to check against real-world Solidity examples.

Notes

  • API changes:
    • isConstant() and evalConstantExpr() are now also accepting VariableDeclaration as an input node.
    • Value type now also includes Buffer (a breaking change).
  • Due to breaking changes we would have to bump major version of the package.

Regards.

@blitz-1306
Tweak logic for better constant expression evaluation (respect file-l…
ac2dfc8 
…evel constants, support more node types and cast operations)
@blitz-1306 blitz-1306 added enhancement New feature or request breaking change Changes that would cause a backward compatibility break labels Oct 6, 2023
@blitz-1306 blitz-1306 requested a review from cd1m0 October 6, 2023 12:34
@blitz-1306 blitz-1306 mentioned this pull request Oct 6, 2023
Open
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2023 •
edited
Loading

Codecov Report

Merging #225 (b9b54ff) into master (f467859) will decrease coverage by 0.16%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   91.81%   91.65%   -0.16%     
==========================================
  Files         267      267              
  Lines        6546     6592      +46     
  Branches     1319     1342      +23     
==========================================
+ Hits         6010     6042      +32     
- Misses        283      288       +5     
- Partials      253      262       +9
Files Coverage Δ
src/types/eval_const.ts 86.27% <75.00%> (-4.16%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@blitz-1306 blitz-1306 mentioned this pull request Oct 9, 2023
Closed
cd1m0
cd1m0 requested changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@cd1m0 cd1m0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Just nits

src/types/eval_const.ts Show resolved Hide resolved
src/types/eval_const.ts Outdated Show resolved Hide resolved
@blitz-1306
Address review remarks: check for OOB when index accessing into a fix…
b9b54ff 
…ed bytes, clamp int to type on cast to bytes.
@blitz-1306 blitz-1306 requested a review from cd1m0 October 10, 2023 05:25
@cd1m0 cd1m0 merged commit 883a401 into master Oct 13, 2023
2 checks passed
@cd1m0 cd1m0 deleted the file-level-const-eval branch October 13, 2023 06:29
@blitz-1306 blitz-1306 mentioned this pull request Oct 16, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cd1m0 cd1m0 cd1m0 approved these changes

Assignees
No one assigned
Labels
breaking change Changes that would cause a backward compatibility break enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

evalLiteral crashes on certain strings Add support for computing file-level constants
3 participants
@blitz-1306 @codecov-commenter @cd1m0